Skip to content

Conversation

@william-xue
Copy link
Member

Checklist

List of tasks you have already done and plan to do.

  • Fix linting errors
  • Tests have been added / updated (or snapshots)

Change information

Describe your modifications here.

Issues

The issues you want to close, formatted as close #1.

Related Links

Links related to this pr.

@vercel
Copy link

vercel bot commented Jun 4, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
varlet ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 27, 2025 2:43am

@vercel
Copy link

vercel bot commented Jun 4, 2025

Someone is attempting to deploy a commit to the varletjs Team on Vercel.

A member of the Team first needs to authorize it.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jun 4, 2025

Open in StackBlitz

@varlet/cli

npm i https://pkg.pr.new/@varlet/cli@1895

@varlet/icons

npm i https://pkg.pr.new/@varlet/icons@1895

@varlet/import-resolver

npm i https://pkg.pr.new/@varlet/import-resolver@1895

@varlet/preset-tailwindcss

npm i https://pkg.pr.new/@varlet/preset-tailwindcss@1895

@varlet/preset-unocss

npm i https://pkg.pr.new/@varlet/preset-unocss@1895

@varlet/shared

npm i https://pkg.pr.new/@varlet/shared@1895

@varlet/touch-emulator

npm i https://pkg.pr.new/@varlet/touch-emulator@1895

@varlet/ui

npm i https://pkg.pr.new/@varlet/ui@1895

@varlet/use

npm i https://pkg.pr.new/@varlet/use@1895

@varlet/vite-plugins

npm i https://pkg.pr.new/@varlet/vite-plugins@1895

commit: 10d37ce

height: 56px;
box-sizing: border-box;
border-bottom: 1px solid rgba(0, 0, 0, 0.05);
color: #333;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

在MD2、MD3暗色主题下不太显眼,也许应该内置在组件中?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

外层虚拟列表容器已经加了
style="height: 448px; background: var(--color-surface-container-low)",
本身就算是放在“容器色”里了。
文本颜色还是 color: #333,和你项目里其它示例基本一致

const containerStyle = computed(() => {
if (props.containerHeight) {
return {
height: typeof props.containerHeight === 'number' ? `${props.containerHeight}px` : props.containerHeight,
Copy link
Member

@rzzf rzzf Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

- height: typeof props.containerHeight === 'number' ? `${props.containerHeight}px` : props.containerHeight,
+ height: `${ toPxNum(props.containerHeight)}px `

使用 toPxNum import { toPxNum } from '../utils/elements'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已参考建议对 containerHeight 做了统一处理,不过这里使用的是 toSizeUnit 而不是 toPxNum:

containerHeight 只是传给 CSS 的高度,不参与内部计算;
使用 toSizeUnit 可以同时支持 number / px 字符串以及其他合法 CSS 长度(vh、% 等),和项目中其它组件的尺寸 props 保持一致;
toPxNum 更适合内部数值计算,且对 % 等值会返回 0,可能导致高度为 0

},
containerHeight: {
type: [Number, String],
default: null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

containerHeight: [Number, String]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已改为使用项目统一的 toSizeUnit 工具处理 containerHeight,和其他组件的尺寸 props 保持一致。

if (!contentRef.value) {
return
}
const nodes = Array.from(contentRef.value.childNodes).filter((node: Node) => node.nodeType === 1) as HTMLElement[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node.nodeType === Node.ELEMENT_NODE

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已将 node.nodeType === 1 替换为 node.nodeType === Node.ELEMENT_NODE,避免 magic number

@william-xue william-xue requested a review from rzzf July 4, 2025 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants